Skip to content

summarize: Don't panic on broken pipe #243

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sharnoff
Copy link

I hit #31 while piping summarize to head. The original issue was in part due to prettytable panicking. That's since been upgraded to a version with the fix, but some of the println!s that happen after table.printstd() will also panic from the broken pipe:

$ summarize summarize ... | head
...
thread 'main' panicked at library/std/src/io/stdio.rs:1165:9:
failed printing to stdout: Broken pipe (os error 32)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

To fix this, change the println!s to writeln!s, and ignore any errors.

Fixes #31.

I hit rust-lang#31 while piping summarize to head. The original issue was in part
due to `prettytable` panicking. That's since been upgraded to a version
with the fix, but some of the `println!`s that happen after
`table.printstd()` will also panic from the broken pipe:

  $ summarize summarize ... | head
  ...
  thread 'main' panicked at library/std/src/io/stdio.rs:1165:9:
  failed printing to stdout: Broken pipe (os error 32)
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

To fix this, change the `println!`s to `writeln!`s, and ignore any
errors.

Fixes rust-lang#31.
@@ -142,7 +142,11 @@ fn diff(opt: DiffOpt) -> Result<(), Box<dyn Error + Send + Sync>> {

table.printstd();

println!("Total cpu time: {:?}", results.total_time);
_ = writeln!(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should just exit the process on a broken pipe rather than keep writing data to nowhere.

Copy link
Author

@sharnoff sharnoff Jun 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense -- implemented a catch-all "exit on failure" in 59876d9.

Do you think it should special-case broken pipe to return a visible error otherwise? or better to just give up

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it should special-case broken pipe to return a visible error otherwise?

Yes, I think it should still show an error and exit when an error other than a broken pipe occurred.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, implemented in 75fde69

@Kobzol
Copy link
Member

Kobzol commented Jul 7, 2025

What do you think about just configuring SIGPIPE to abort the process? For example like this. I think that's a more universal solution than patching a few print lines.

@ChrisDenton
Copy link
Member

Non-unix platforms may not have signals (e.g. windows).

@Kobzol
Copy link
Member

Kobzol commented Jul 7, 2025

Right, but I thought that this issue only happens on Unix? Does Rust also somehow ignore sigpipe (or something similar) on Windows? Is that even a thing?

@ChrisDenton
Copy link
Member

Closing a pipe on Windows just causes further writes to fail. The println macro will panic on any failure to write so it will panic on a closed pipe.

@Kobzol
Copy link
Member

Kobzol commented Jul 7, 2025

I see. Well, hard to say how many people redirect CLI commands into pipes on Windows with summarize 😆

@ChrisDenton
Copy link
Member

ChrisDenton commented Jul 7, 2025

Sure, I don't know how much of an issue it is in practice.

Incidentally rustc has a safe_print macro for handling this across platforms. It still panics but with a payload that tells rustc to treat it as a normal (albeit fatal) error rather than an unexpected panic (this works because rustc uses catch_unwind to catch all panics).

@bjorn3
Copy link
Member

bjorn3 commented Jul 7, 2025

Why does println!() even produce a panic message for broken pipes? Can't it just silently panic?

@ChrisDenton
Copy link
Member

"silently panic"?

@bjorn3
Copy link
Member

bjorn3 commented Jul 7, 2025

std::panic::resume_unwind(Box::new(SomeMarkerType)) will unwind without producing any panic message.

@ChrisDenton
Copy link
Member

Ah. Likely because nobody has proposed it to libs-api yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Summarize panics if piped into head
4 participants